update(gvisor): implement get_stats for gVisor #463
Conversation
poiana
added
release-note-none
kind/feature
|
Hi @loresuso. Thanks for your PR. I'm waiting for a falcosecurity member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
| @@ -168,6 +171,8 @@ class engine { | |||
| std::string m_root_path; | |||
| std::string m_trace_session_path; | |||
|
|
|||
| scap_stats m_stats; | |||
There was a problem hiding this comment.
nit: there's one tiny thing that I'm not completely happy with about this stats structure, it's the fact that it is not always updated. the m_drops is only updated if the get_stats() function is called. To make the invariant hold I would suggest the following: only save three fields in the class, that would be the number of drops on gvisor side (perhaps with a descriptive name like m_stat_drops_gvisor so we remember what it its), the number of drops because of parsing errors and the total number of events processed. This way we can fill only the relevant fields in the stats structure when requested and we don't need to maintain an m_drops that isn't updated and a bunch of zeroes for fields that are not relevant for gvisor.
There was a problem hiding this comment.
Yeah that would make sense, ie: internally use the real gvisor_stats, but expose to the world the expected scap_stats; i like it.
Aside from this, it LGTM!
There was a problem hiding this comment.
Agreed! I am working on it, will ping you soon :)
|
This is very useful especially for early adopeters of this functionality! I would add it to release. 🚀 left a minor comment. |
Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>
|
I have also noticed that the way we are dealing with |
|
Mmmh i think that no impl is better than buggy impl, given the time constraints too :) |
Signed-off-by: Lorenzo Susini <susinilorenzo1@gmail.com>
|
Uh sorry @loresuso misread your previous sentence as "i have no idea about how to solve" :D |
|
No problem Fede! Idea is to keep a |
|
This makes sense, thanks! |
|
@LucaGuerra: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
LGTM label has been added. Git tree hash: 3ca6093f6fb2d930cd678d0b89d8159278c211a0
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Andreagit97, FedeDP, loresuso The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Lorenzo Susini susinilorenzo1@gmail.com
What type of PR is this?
/kind feature
Any specific area of the project related to this PR?
/area libscap-engine-bpf
What this PR does / why we need it:
This PR implements the
get_statsvtable function for gVisor, letting Falco be able to retrieve statistics like the number of drops or events.Statistics are collected in a
scap_statsstructure like usual. To be precise:n_drops_bugis used to count the number of dropped events due to parsing errorsn_drops_buffersis used to count the number of dropped events from gVisor side. This number can be retrieved from the header of each message.n_dropsis the total number of dropped events, so it is computed as the sum of the previous two.n_evtscounts the number of events that were successfully returned bynextWhich issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: